Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CloudStack modules: rename _facts -> _info #61090

Merged
merged 17 commits into from Aug 29, 2019
Merged

CloudStack modules: rename _facts -> _info #61090

merged 17 commits into from Aug 29, 2019

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Aug 22, 2019 •
edited

SUMMARY

Fixes #60515, fixes #53916.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cs_instance_facts
cs_zone_facts

@felixfontein felixfontein mentioned this pull request Aug 22, 2019
Closed
@ansibot
Copy link
Contributor

ansibot commented Aug 22, 2019

cc @acozine @dpassante @jeffersongirao @mattclay @resmo @rhtyd
click here for bot help

@ansibot

This comment has been minimized.

Sign in to view
@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. cloud cloudstack deprecated This issue/PR relates to a deprecated module. docs This issue/PR relates to or includes documentation. has_issue module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Aug 22, 2019
@resmo
Copy link
Contributor

resmo commented Aug 22, 2019

I though about changing the modules to return lists: list of zones, list of instances, with the possibility to filter by name to only get one item (but still a list). @dpassante any thoughts?

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Aug 22, 2019
samccann
samccann approved these changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs portion LGTM

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 28, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 28, 2019
@dpassante
Copy link
Contributor

@resmo I think it would be an interesting feature. However, returning a list for filtered items may break existing deployments that expect a dictionary.
These impacts could be mitigated by keeping the cloudstack_zone / cloudstack_instance exposure in the ansible_facts when there is only one item.

I also thought about writing a meta module that exposes lists of resources with the ability to filter by type of resources and by name to not break the existing but it seemed redundant with other modules and time-consuming to write and to maintain.

@dpassante
Copy link
Contributor

@felixfontein Indeed. So, it's a good time to change the values returned by the new _info module!

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 29, 2019
@felixfontein
Copy link
Contributor Author

@dpassante @resmo PTAL!

@felixfontein
Copy link
Contributor Author

CS tests run through. Sanity tests should be fixed with the last commits.

ready_for_review

dpassante
dpassante approved these changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@dpassante dpassante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/ansible/modules/cloud/cloudstack/cs_zone_info.py Show resolved Hide resolved
lib/ansible/modules/cloud/cloudstack/cs_instance_info.py Show resolved Hide resolved
@resmo
Copy link
Contributor

resmo commented Aug 29, 2019

shipit

@resmo
Copy link
Contributor

resmo commented Aug 29, 2019

Fixes #53916

@resmo resmo mentioned this pull request Aug 29, 2019
Closed
@felixfontein
Copy link
Contributor Author

Final commits. Fix version number for deprecation and add examples.

@resmo
Copy link
Contributor

resmo commented Aug 29, 2019

shipit

@dpassante
Copy link
Contributor

Thanks @felixfontein for these changes.

@dpassante
Copy link
Contributor

shipit

@felixfontein felixfontein merged commit 0c592c8 into ansible:devel Aug 29, 2019
@felixfontein felixfontein deleted the info-cloudstack branch August 29, 2019 14:15
@felixfontein
Copy link
Contributor Author

@samccann @s-hertel @dpassante @resmo thank you all for helping getting this done! :)

adharshsrivatsr pushed a commit to adharshsrivatsr/ansible that referenced this pull request Sep 3, 2019
@felixfontein @adharshsrivatsr
CloudStack modules: rename _facts -> _info (ansible#61090)
3426492 
* Rename cloudstack _facts -> _info

* Add changelog.

* Fix errors due to overloaded get_result having different parameter names.

* Fix examples.

* Change debug tasks.

* Remove unneeded code.

* Change from rename -> deprecate+new module.

* Make cs_zone_info return a list.

* Make cs_instance_info return a list.

* Fix return value docs.

* Fix typo.

* Improve tests.

* Fix cs_zone_info.

* Linting.

* Fix alias/option switch.

* Fix version numbers.

* Extend docs.
anas-shami pushed a commit to anas-shami/ansible that referenced this pull request Sep 23, 2019
@felixfontein @anas-shami
CloudStack modules: rename _facts -> _info (ansible#61090)
e061559 
* Rename cloudstack _facts -> _info

* Add changelog.

* Fix errors due to overloaded get_result having different parameter names.

* Fix examples.

* Change debug tasks.

* Remove unneeded code.

* Change from rename -> deprecate+new module.

* Make cs_zone_info return a list.

* Make cs_instance_info return a list.

* Fix return value docs.

* Fix typo.

* Improve tests.

* Fix cs_zone_info.

* Linting.

* Fix alias/option switch.

* Fix version numbers.

* Extend docs.
@ansible ansible locked and limited conversation to collaborators Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-hertel s-hertel s-hertel left review comments

@dpassante dpassante dpassante approved these changes

@samccann samccann samccann approved these changes

Assignees

@resmo resmo

Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. cloud cloudstack deprecated This issue/PR relates to a deprecated module. docs This issue/PR relates to or includes documentation. has_issue module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
6 participants
@felixfontein @ansibot @resmo @dpassante @s-hertel @samccann